-
Notifications
You must be signed in to change notification settings - Fork 747
autostart: Ensure instance is started/stopped by launchd or systemctl
#4139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
783c0f2 to
428fcb1
Compare
|
How to test this PR? |
|
Also please explain what issue is being solved by this PR |
opened #4141 |
ac9db28 to
31d7007
Compare
89c500c to
7082f48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be *_linux.go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code that is platform-dependent and can only be built on Linux is already isolated in systemd_linux.go. systemd.go has code that can be built on any platform, only the operation is platform-dependent.
If we put the code itself included in systemd.go into systemd_linux.go, I think the number of dummy codes required for systemd_others.go will increase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be *_darwin.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code included in launchd.go is based on the darwin platform, but the build is not based on the darwin platform.
The runtime guard should be enough.
7082f48 to
994efee
Compare
I was researching how to test and took a detour to fix |
994efee to
f3861a0
Compare
2af6c71 to
497f9ed
Compare
497f9ed to
4a9a91e
Compare
4a9a91e to
89ec621
Compare
jandubois
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, didn't have time to do a proper review yet, so this is more like first-impressions from browsing the code.
I also feel like the OS-specific code could be separated better into files that compile only on the respective platforms. Especially the manager should not have references to Launchd or Systemd in it, but work generically via an interface.
| // Network reconciliation will be performed by the process launched by the autostart manager | ||
| if registered, err := autostart.IsRegistered(ctx, inst); err != nil && !errors.Is(err, autostart.ErrNotSupported) { | ||
| return fmt.Errorf("failed to check if the autostart entry for instance %q is registered: %w", inst.Name, err) | ||
| } else if !registered { | ||
| err = reconcile.Reconcile(ctx, inst.Name) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic seems to be repeated in several places. Should it be part of reconcile.Reconcile() instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are several reconcile.Reconcile() calls that are not related to autostart.IsRegistered().
It is not good to put it in reconcile.Reconcile().
| logrus.Infof("The autostart file %q has been created or updated", autostart.GetFilePath(runtime.GOOS, inst.Name)) | ||
| if registered, err := autostart.IsRegistered(ctx, inst); err != nil { | ||
| return fmt.Errorf("failed to check if the autostart entry for instance %q is registered: %w", inst.Name, err) | ||
| } else if startAtLogin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use else when the if branch ends in a return. @alexandear I'm surprised we don't have a linter rule enabled for this.
| } else if startAtLogin { | |
| } | |
| if startAtLogin { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The linter rule should be subject to return in the last else block.
pkg/autostart/managers.go
Outdated
| var Launchd = &TemplateFileBasedManager{ | ||
| filePath: launchd.GetPlistPath, | ||
| template: launchd.Template, | ||
| enabler: launchd.EnableDisableService, | ||
| autoStartedIdentifier: launchd.AutoStartedServiceName, | ||
| requestStart: launchd.RequestStart, | ||
| requestStop: launchd.RequestStop, | ||
| } | ||
|
|
||
| // Systemd is the autostart manager for Linux. | ||
| var Systemd = &TemplateFileBasedManager{ | ||
| filePath: systemd.GetUnitPath, | ||
| template: systemd.Template, | ||
| enabler: systemd.EnableDisableUnit, | ||
| autoStartedIdentifier: systemd.AutoStartedUnitName, | ||
| requestStart: systemd.RequestStart, | ||
| requestStop: systemd.RequestStop, | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels weird. Why is TemplateFileBasedManager not a regular Go interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TemplateFileBasedManager is one of the implementations of the type autoStartManager interface.
The differences between platforms are divided into pkg/autostart/launchd and pkg/autostart/systemd.
TemplateFileBasedManager has a common part that is not platform-dependent.
| return err | ||
| if registered, err := autostart.IsRegistered(ctx, inst); err != nil && !errors.Is(err, autostart.ErrNotSupported) { | ||
| return fmt.Errorf("failed to check if the autostart entry for instance %q is registered: %w", inst.Name, err) | ||
| } else if !registered { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| } else if !registered { | |
| } | |
| if !registered { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To do that, it is necessary to expand the scope of the registered and err variables.
If it is a variable that is necessary outside of if... {...} else if ... {...}, I will expand the scope, but if the scope of use is only this place, I don't want to expand the scope.
d9a7159 to
53ab3e5
Compare
The parts where the build depends on the platform have already been separated. Although it is platform-dependent at the time of execution, the logic itself is platform-independent, so unit testing is currently possible across platforms. If it is changed so that it is built only on the corresponding platform, defects that can be found in unit tests on macOS will only be found on CI's Linux. |
- Hide creating/deleting an autostart entry file into `Register`/`Unregister` as the implementation details. - Separate the `launchd` and `systemd`-specific code into packages. Signed-off-by: Norio Nomura <[email protected]>
…ctl` If the instance isn’t launched by `launchd`, it won’t be stopped on log out. This changes to use `launchctl` or `systemctl` to start/stop the instance if it’s registered to autostart. ## Affected sub commands: - `limactl start` - `limactl stop` - `limactl restart` - `limactl edit` - `limactl shell` ## API changes ### `pkg/autostart` - Introduced `AutoStartedIdentifier()`: - If not empty, it indicates whether the instance was started by `launchd` or `systemd`. - Added `RequestStart()`: - Delegates the operation to `launchd` or `systemd`. - Added `RequestStop()`: - Delegates the operation to `launchd` or `systemd`. ### `pkg/autostart/launchd` - Added `AutoStartedServiceName()`: - Uses the XPC_SERVICE_NAME environment variable as the service name. - Added `RequestStart()`: - Uses `launchctl enable service-target` to avoid failing `bootstrap`. - Uses `launchctl bootstrap domain-target plist-path`. - Added `RequestStop()`: - Uses `launchctl bootout service-target` if the instance is launched by `launchd`. - Added `--progress` to the `limactl` option in `io.lima-vm.autostart.INSTANCE.plist`: - Required to support `limactl start --progress`. ### `pkg/autostart/systemd` - Added `AutoStartedServiceName()`: - Uses `CurrentUnitName()` by `github.com/coreos/go-systemd/v22/util` as the service identifier. - Added `RequestStart()`: - Uses `systemctl --user start unit-name`. - Added `RequestStop()`: - Uses `systemctl --user stop unit-name` if the instance is launched by `systemd`. - Added `--progress` to the `limactl` option in `[email protected]`: - Required to support `limactl start --progress`. ### `pkg/hostagent` - Add `AutoStartedIdentifier` to `Info`. - If not empty, it indicates whether the instance was started by `launchd` or `systemd`. ### `pkg/instance` - `StartWithPaths()`: - Use `autostart.IsRegistered()` to check if the instance is registered to autostart. - If `launchHostAgentForeground` is true, ignore autostart registration. - If the instance is registered to autostart, use `autostart.RequestStart()` instead of launching HostAgent. - `StopGracefully()`: - Use `autostart.RequestStop()`. - `Restart()`, `RestartForcibly()`: - Use `autostart.IsRegistered()` to skip `networks.Reconcile()` if the instance is registered to autostart. Signed-off-by: Norio Nomura <[email protected]> bump `coreos/go-systemd` to v22.6.0 Signed-off-by: Norio Nomura <[email protected]> # Conflicts: # cmd/limactl/edit.go # cmd/limactl/shell.go # cmd/limactl/start.go # pkg/instance/restart.go
…hctl bootstrap` Because instances may be stopped without unloading the plist file. e.g. `limactl stop -f` or `limactl factory-reset`. If the plist file is not unloaded, `launchctl bootstrap` will fail. Signed-off-by: Norio Nomura <[email protected]>
Signed-off-by: Norio Nomura <[email protected]>
- Remove wrong INFO message - Remove the `—progress` option. Signed-off-by: Norio Nomura <[email protected]>
53ab3e5 to
90514e7
Compare
Signed-off-by: Norio Nomura <[email protected]>
90514e7 to
1613e09
Compare
Separated OS-specific code. |
AkihiroSuda
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
|
Thanks! 🙏🏻 |
This PR is formed with two parts:
autoStartManagerto prepare for the following commitlaunchdorsystemctlRefactor to add
autoStartManagerRegister/Unregisteras the implementation details.launchdandsystemd-specific code into packages.Use
launchctlorsystemctlto start/stop the instance if it’s registered to autostart.If the instance isn’t launched by
launchd, it won’t be stopped on log out.Affected sub commands:
limactl startlimactl stoplimactl restartlimactl editlimactl shellAPI changes
pkg/autostartAutoStartedIdentifier():launchdorsystemd.RequestStart():launchdorsystemd.RequestStop():launchdorsystemd.pkg/autostart/launchdAutoStartedServiceName():RequestStart():launchctl enable service-targetto avoid failingbootstrap.launchctl bootstrap domain-target plist-path.RequestStop():launchctl bootout service-targetif the instance is launched bylaunchd.--progressto thelimactloption inio.lima-vm.autostart.INSTANCE.plist:limactl start --progress.pkg/autostart/systemdAutoStartedServiceName():CurrentUnitName()bygithub.com/coreos/go-systemd/v22/utilas the service identifier.RequestStart():systemctl --user start unit-name.RequestStop():systemctl --user stop unit-nameif the instance is launched bysystemd.--progressto thelimactloption in[email protected]:limactl start --progress.pkg/hostagentAutoStartedIdentifiertoInfo.launchdorsystemd.pkg/instanceStartWithPaths():autostart.IsRegistered()to check if the instance is registered to autostart.launchHostAgentForegroundis true, ignore autostart registration.autostart.RequestStart()instead of launching HostAgent.StopGracefully():autostart.RequestStop().Restart(),RestartForcibly():autostart.IsRegistered()to skipnetworks.Reconcile()if the instance is registered to autostart.